Remove PyQt4 and PySide support #512
Conversation
Note - do not try to fix typos while managing small children.
Codecov Report
@@ Coverage Diff @@
## master #512 +/- ##
==========================================
- Coverage 36.94% 36.84% -0.11%
==========================================
Files 470 470
Lines 25997 25921 -76
Branches 3955 3931 -24
==========================================
- Hits 9604 9550 -54
+ Misses 15969 15955 -14
+ Partials 424 416 -8
Continue to review full report at Codecov.
|
|
I think this is ready to go. Willing to be told to split this into two PRs, one fixing the wxPython issues and one fixing the Pyface issues. |
| @@ -33,14 +21,7 @@ | |||
| __version__ = QT_VERSION_STR | |||
| __version_info__ = tuple(map(int, QT_VERSION_STR.split("."))) | |||
|
|
|||
|
|
|||
| elif qt_api == "pyside2": | |||
| else: | |||
There was a problem hiding this comment.
Just noting an observation: It is not necessary (though would be defensive) to check qt_api is "pyside2" here again because pyface.qt.__init__ already ensures qt_api must be one of "pyside2" and "pyqt5". I also noticed that the order of these if-else branches are consistent with the reverse order of QtAPIs.
There was a problem hiding this comment.
I think it is OK to do this, since Pyside2 is the blessed Python Qt wrapper provided by the Qt Company. If a new wrapper cam along it would likely follow the Pyside conventions more than the PyQt conventions.
| @@ -18,17 +18,6 @@ | |||
| from pyface.base_toolkit import Toolkit | |||
| from .gui import GUI | |||
|
|
|||
| if qt_api == "pyqt": | |||
There was a problem hiding this comment.
🤖 flake ❄️ : I can't see qt_api QtCore being used anywhere... So they are now unused imports?
There was a problem hiding this comment.
I'll check. There are a few things done in the init.py to make sure that we have a valid environment and Qt has been initialized to a working state. It is possible QtCore is imported just to prove it can be.
There was a problem hiding this comment.
Nope - it's redundant. Will remove.
| "pyside2": {"ETS_TOOLKIT": "qt4", "QT_API": "pyside2"}, | ||
| "pyqt": {"ETS_TOOLKIT": "qt4", "QT_API": "pyqt"}, | ||
| "pyqt5": {"ETS_TOOLKIT": "qt4", "QT_API": "pyqt5"}, | ||
| "pyqt": {"ETS_TOOLKIT": "qt4", "QT_API": "pyqt5"}, |
There was a problem hiding this comment.
Nit and subjective: I would prefer seeing "pyqt5" in the environment name so that wherever I see "pyqt" in CI logs etc I know it is the old Qt4 being used.
Unrelated drive-by comment: This ETS_TOOLKIT looks sadly inconsistent :( "qt4" does not mean "qt4" anymore, it just means Qt.
There was a problem hiding this comment.
Post-mortem: It is painful when an old environment name is reused for a new meaning and then that change has to be reverted later. Let's not do this again.
| @@ -302,7 +302,7 @@ def _focus_changed(self, old, new): | |||
| # It is possible for the C++ layer of this object to be deleted between | |||
| # the time when the focus change signal is emitted and time when the | |||
| # slots are dispatched by the Qt event loop. This may be a bug in PyQt4. | |||
| if qt_api == "pyqt": | |||
There was a problem hiding this comment.
This branch only runs for PyQt but not PyQt5, now it runs for PyQt5 too, is this intended?
There was a problem hiding this comment.
Yes. It's harmless for PyQt5 if the mentioned bug has been fixed, but it will protect if it has not.
mdickinson
changed the title
|
I retitled the PR; the original title misled me, since it suggested that this was just about CI changes, not about user-facing changes. |
This reverts commit b149f5f.
A start along the road to do #510. This removes PyQt4 and PySide support from etstool, Travis and Appveyor. Drive-by clean-up fixes mock dependencies as well and fixes unrelated wxPython CI failures from new wxPython release.